Comment parser #11252
Comment parser #11252
Conversation
This token is not needed, we will later use the position information to pad each token.
... which is not handling at all for the time being
There was a problem hiding this comment.
Thank you for taking on this work. I think adding a Comment constructor to Field is not a good design. It modifies the meaning of the type (and indeed this forces you to make functions like elementInLayoutContext return [Fields]).
I think this has been already discussed before: we have the ann parameter which can be used to keep hold on the comments. E.g.
data Comment ann = Comment !ann !ByteString
type FieldWithComments ann = Field ([Comment ann], ann)In this design each Field carries the comments preceding it, annotated with their position. An extra annotation marks the position of the field itself. Any comment at the end of the file would need to be captured separately.
This is already the practice of few packages developed by the community. Is there a reason to deviate from this?
| -- elementInLayoutContext ::= ':' fieldLayoutOrBraces | ||
| -- | arg* sectionLayoutOrBraces | ||
| elementInLayoutContext :: IndentLevel -> Name Position -> Parser (Field Position) | ||
| elementInLayoutContext :: IndentLevel -> Name Position -> Parser [Field Position] |
There was a problem hiding this comment.
The comment suggest this function parses one field but the signature is changed to return a list of fields.
| fieldLayoutOrBraces :: IndentLevel -> Name Position -> Parser (Field Position) | ||
| -- fieldLayoutOrBraces ::= '\\n'? '{' comment* (content comment*)* '}' | ||
| -- | comment* line? comment* ('\\n' line comment*)* | ||
| fieldLayoutOrBraces :: IndentLevel -> Name Position -> Parser [Field Position] |
|
Here's a preliminary benchmark done with Upstream: ~/r/haskell/cabal λ lts-18.28
$ hyperfine --runs 30 './validate.sh --partial-hackage-tests'
Benchmark 1: ./validate.sh --partial-hackage-tests
Time (mean ± σ): 203.400 s ± 21.816 s [User: 150.484 s, System: 39.487 s]
Range (min … max): 183.805 s … 277.905 s 30 runsThis branch: …/wt/haskell/cabal/exact-pp-leana λ lts-18.28
$ hyperfine --runs 30 './validate.sh --partial-hackage-tests'
Benchmark 1: ./validate.sh --partial-hackage-tests
Time (mean ± σ): 199.168 s ± 10.540 s [User: 156.373 s, System: 39.818 s]
Range (min … max): 184.443 s … 242.563 s 30 runsThank you for your response Andrea, I'll write up a response and get back to you soon :) |
|
Thank you for your comment @andreabedini :)
That looks very interesting, but how would I deal with files that are just comments? To the point of view of readFields they should be valid yet we would have no I do think your model is very interesting so if you have the time to, please show working PR against mine so we can simply merge it in 🙏
Could you elaborate which packages are these? I would love to have more insight on how people solve similar problems. Are there other design issues that needs to be addressed ? |
Are they valid Cabal files if there is nothing but comments? I don't think so.
For instance, annotateFieldsWithSource :: ByteString -> [Field Position] -> [Field ByteString]which annotates each field with its source including all adjacent comments. It can be modified to return |
| match _ = Nothing | ||
|
|
||
| -- | Collect comments into a map. The second field of the output will have no comment | ||
| extractComments :: Ord ann => [Field ann] -> (Map.Map ann ByteString, [Field ann]) |
There was a problem hiding this comment.
Could you possibly outline how the output of this function is supposed to be used? Now that we detached comments from fields, how do we reconstruct the original document? How do we do it if [Field ann] is programmatically updated (say, adding or removing elements)?
There was a problem hiding this comment.
Could you possibly outline how the output of this function is supposed to be used?
This function recursively extracts the comments from the fields (the updated design variant does the same thing, albeit being more concise).
It is used to extract the comments out right before running the parseFieldGrammar parser in src/Distribution/PackageDescription/Parsec.hs. This allows us to not change the grammar and be able to directly inject the comments into the GenericPackageDescription.
Now that we detached comments from fields, how do we reconstruct the original document?
We can't yet. Following Jappie's proposal, we need to store the exact position of each field as a map (called exactPositions) indexed by the path in the rose tree (typed [NameSpace], a list of path segments). And then we will have the right information to construct the exact printer.
How do we do it if [Field ann] is programmatically updated (say, adding or removing elements)?
To quote Jappie:
The issue for addition is that you now have to invent exact positions. for removal, if it involves a line, you've to fix up all following lines, (and it has to know something was removed).
I haven't thought about this thoroughly because it's out of scope of this PR, but it should be very much feasible.
Pretty sure you need at minimum |
|
Hi friends, thanks for all your responses. Leana needs some time to read up on the exact proposal to see how it all fits together before replying. |
|
Cabal is one of the toughest code bases I ever worked on, so I'm quite amazed by Leana making progress so quickly! |
We need to look into how to wire the output for it to hold the comments in the right position.
|
Thank you Bodigrim and Jappie for your kind words! That means a lot to me, I'm glad to be on the right track. I have started (and completed) to rewrite my PR using Andrea's approach.
Good to know. Currently the top level parser drops the comments consumed if there are no fields to attach them to. Let me know what you think about the change :) |
This fixes builds for old GHC
|
Here are the benchmark results. The baseline has been rerun because I did these ones on a VPS machine, and they are not comparable to the last ones I ran on my machine. # baseline
leana@Ubuntu-2404-noble-amd64-base:~/cabal$ hyperfine './validate.sh --partial-hackage-tests'
Benchmark 1: ./validate.sh --partial-hackage-tests
Time (mean ± σ): 253.353 s ± 9.520 s [User: 196.642 s, System: 60.881 s]
Range (min … max): 241.649 s … 271.207 s 10 runs
# this PR
leana@Ubuntu-2404-noble-amd64-base:~/cabal$ hyperfine --setup "./validate.sh --partial-hackage-tests" "./validate.sh --partial-hackage-tests"
Benchmark 1: ./validate.sh --partial-hackage-tests
Time (mean ± σ): 253.163 s ± 7.451 s [User: 196.432 s, System: 58.507 s]
Range (min … max): 239.373 s … 266.656 s 10 runs |
In my prototype I have replaced Where the extra annotation is for anything coming after the last field.
In addition to @Bodigrim's
I am available to discuss and support her effort. @leana8959 I'll reach out privately.
I warmly second this! |
5 participants
This is the first part of the exact print parser. In this PR I changed the lexer so instead of dropping the comments it emits them to the parser which is further stored in
GenericPackageDescription.Please let me know your thoughts!
Checklist below:
This PR modifies behaviour or interface
Include the following checklist in your PR:
significance: significantin the changelog file.